-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix instruction for RISC-V (the previous instruction didn't work) #10374
Fix instruction for RISC-V (the previous instruction didn't work) #10374
Conversation
@aaronfranke Please review this whenever you get the chance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like either of the changes in this PR as-is.
- Clang. RISC-V GCC has | ||
`bugs with its atomic operations <https://github.com/riscv-collab/riscv-gcc/issues/15>`__ | ||
which prevent it from compiling Godot correctly. Any version of Clang from 16.0.0 upwards | ||
will suffice. Download it from the package manager of your distro, and make sure that | ||
it *can* compile to RISC-V. You can verify by executing this command ``clang -print-targets``, | ||
make sure you see ``riscv64`` on the list of targets. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't remove this section for telling the user to install Clang, even if they can use the toolchain-provided one. The system clang worked for me, and there is no harm in having it. It's also important to tell the user that GCC does not work, and telling the user how to check if Clang supports the target.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the Clang dosn't work for me! It may be matter on the version you're using. What versions of Clang and riscv-gnu-toolchain do you use?
My versions:
- Clang 18.1.8
- riscv-gnu-toolchain 2023.06.09
- Mold 2.34.1
Also a small note about riscv-gnu-toolchain: the official Godot documentation says: "the older the toolchain, the more compatible our final binaries will be", and recommends installing version 2021.12.22-ubuntu-18.04. But I installed version 2023.06.09-ubuntu-20.04, since this is the oldest version with which I was able to compile Godot. For newer ones I get the error
mold: fatal: huf_decompress_amd64.linuxbsd.editor.rv64.llvm.o: incompatible file type: riscv64 is expected but got x86_64
Most likely, a more correct way to install the necessary tools exists (I suspect, related to configuring Clang). It should also be noted that I did not check the functionality of the resulting Godot builds, and given the content of the forms on this topic, there is a high chance that they won't even start.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rest of the comments make sense and I fully agree with them. I will make corrections regarding the gcc and correct the command for the terminal.
But I still don't know what to say about Clang. May be you have an idea for appropriate wording that will not confuse the reader?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For RISC-V, this platform is so new that the advice about compiling for older Ubuntu versions for better compatibility is superseded by the need to use newer systems for including critical bugfixes. I have upgraded my Ubuntu systems to 24.04 since I last tried this, and I think that is fine as a baseline, given that RISC-V devices are still to this day very niche and will be for several years.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have upgraded my Ubuntu systems to 24.04 since I last tried this, and I think that is fine as a baseline, given that RISC-V devices are still to this day very niche and will be for several years.
Ok, so... No changes on this page needed?
I still think that if the toolchain have it's own Clang with which it works fine insted of newer one version of Clang we shouldn't insist on installing different Clang. I don't know how it works, but I have an issue with it. Otherwise we need to add a comment that the user should rely on newer versions of Linux (which is already contrary to the documentation and makes it more complicated for user).
I made changes on all other claims in my PR. Maybe we could add such note in page:
"You can use a self-installed version of Clang, but it may have compatibility issues with riscv-gnu-toolchain."
Will this be enough? I'll wait for your opinion.
|
||
:: | ||
|
||
export PATH="$RISCV_TOOLCHAIN_PATH/bin:$PATH" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you've mentioned in the warning below, exporting this into your PATH may break things. We shouldn't encourage users to write this command, even just in the terminal. Instead, we should override the variable just for the command, instead of exporting it into the PATH, like this:
PATH="$RISCV_TOOLCHAIN_PATH/bin:$PATH" scons arch=rv64 use_llvm=yes linker=mold ...
Adjusting the code block below to include this at the start would be welcome.
Corrected in accordance with the discussion.
|
||
:: | ||
|
||
PATH="$RISCV_TOOLCHAIN_PATH/bin:$PATH" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting this will only work if it's a part of the command as a prefix to it. Otherwise, it will only be changed in the shell itself. https://stackoverflow.com/questions/1158091/defining-a-variable-with-or-without-export
The command below should be edited to include this, it doesn't need its own separate code block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me except for one minor issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only two minor changes need to be made. This should be good to merge after that.
godotengine#10374 (comment) Co-authored-by: Matthew <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grammar and spelling looks good to me.
Thanks! And congrats on your first merged PR! |
I checked this for Godot 4.3 sources on a Virtual Machine with Ubuntu 22.04.3(LTE). This pull request is the only instruction that worked for me. In all other cases I get an error.
I came to the conclusion that the user does not need to install clang at all to compile for RISC-V, since clang comes with the tool specified in the documentation. Moreover, only this toolset can be used for compilation. Check out details of my experiment:
As you can see below, I have clang installed (version 18.1.8, much better then 16-minimum mentioned in documentation). When I try to use command from actual version of "Compiling for Linux, *BSD" page, I get an error at the end of Godot compilation. ( "scons platform=linuxbsd use_llvm=yes linker=lld" works well therefore it's not problem with my clang )
Now I gonna uninstall official clang and check the same scons command after adding $RISCV_TOOLCHAIN_PATH/bin into PATH. Let's uninstall clang and llvm via apt-get purge command. To check that clang were trully uninstalled I trying to check it's version again.
(here I open a new terminal)
As you can see, now my Linux environment use built-in clang from riscv-gnu-toolchain path and it compiles Godot without errors. But, of course, adding $RISCV_TOOLCHAIN_PATH/bin into PATH would automatically "replaces" the installed Clang with another one if you have it on your system. The same is true vice versa — if the user installs a separate Сlang, then when trying to compile for RISC-V, an inappropriate version can be selected. That's why I recommend not to make adding to PATH permanent and do it every time before RISC-V-compiling.
In actual page version said: "RISC-V GCC has bugs with its atomic operations which prevent it from compiling Godot correctly". I can not prove that Godot compiled in my case have no such bugs, since I don't have any RISC-V devices. This requires testing (I attached the compiled file at the end of this comment). But, as I understang, GCC and Clang are different compilers, and I don't use GCC in suggested instruction.
godot.linuxbsd.editor.rv64.llvm